Fix Predicate Pushdown: split_members should be able to split aliased predicate #1368

viirya · 2021-11-26T17:55:37Z

Which issue does this PR close?

Closes #1367.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

viirya · 2021-11-26T18:07:23Z

cc @Dandandan

Dandandan · 2021-11-27T10:31:44Z

datafusion/src/optimizer/constant_folding.rs

        Projection: #test.a\
-        \n  Filter: NOT #test.c AS test.c = Boolean(false)\
-        \n    Filter: #test.b AS test.b = Boolean(true)\
+        \n  Filter: NOT #test.c\


Dandandan · 2021-11-27T10:47:45Z

thanks @viirya for the quick change / PR! I think this is a good change.

I am wondering if there's more places where we don't want to add an alias.

Also we probably need some more tests to cover filter pushdown in combination with other optimization passes like this one.

Another thing to consider might be for the filter operation to ignore the alias.

FYI @alamb WDYT

alamb

I don't really understand how aliases can mess up filter push down (perhaps filter pushdown should be stripping off the aliases)?

alamb · 2021-11-27T11:34:50Z

datafusion/src/optimizer/constant_folding.rs

-                            } else {
-                                Ok(new_e)
+                        match plan {
+                            LogicalPlan::Filter { .. } => Ok(new_e),


I would like to recommend adding a comment here about why we don't add aliases for filter.

alamb · 2021-11-27T11:38:57Z

Yeah, I suggest:

Adding a test with the TPCH explain plans (so we can detect such errors earlier in the future)
Looking into what is going on with Filter pushdown that aliases disabled them.

I "feel" like there should be nothing wrong with aliasing the output of a Filter

viirya · 2021-11-28T07:28:38Z

Thanks. I'm adding a test for TPCH plans. I will take a look on Filter pushdown with aliases later.

viirya · 2021-11-28T08:39:27Z

Currently the filter_push_down rule fails on TPCH q10 with aliased on Filter because split_members method doesn't address Alias so a predicate like #orders.o_orderdate >= Date32("8674") AND #orders.o_orderdate < Date32("8766") AND #lineitem.l_returnflag = Utf8("R") AS ... will not be split so it won't be pushed down.

By adding Alias into split_members, the rule can push down aliased predicate.

This reverts commit f07cbdd.

Dandandan · 2021-11-28T12:52:11Z

datafusion/tests/sql.rs

 }

+#[tokio::test]
+async fn tpch_explain_q10() -> Result<()> {


Nice 👍
Maybe it would be good to have those in the tpch crate instead?
In that case we could include some / all other queries as well.

I agree -- filed #1377 to track this

alamb

Thank you @viirya !

Would it be possible to move the tpch tests into the tpch crate, as suggested by @Dandandan as part of this PR?

Someone can add explain plans for the other TPCH queries as part of #1377 so I don't think that is necessary.

Thanks for sticking with this one and finding the elegant solution

alamb · 2021-11-28T14:05:18Z

datafusion/src/optimizer/filter_push_down.rs

            split_members(left, predicates);
            split_members(right, predicates);
        }
+        Expr::Alias(expr, _) => {


💯 excellent

alamb · 2021-11-28T14:10:03Z

datafusion/tests/sql.rs

 }

+#[tokio::test]
+async fn tpch_explain_q10() -> Result<()> {


I agree -- filed #1377 to track this

alamb · 2021-11-28T14:10:56Z

datafusion/tests/sql.rs

+    \n          Join: #customer.c_custkey = #orders.o_custkey\
+    \n            TableScan: customer projection=Some([0, 1, 2, 3, 4, 5, 7])\
+    \n            Filter: #orders.o_orderdate >= Date32(\"8674\") AND #orders.o_orderdate < Date32(\"8766\")\
+    \n              TableScan: orders projection=Some([0, 1, 4]), filters=[#orders.o_orderdate >= Date32(\"8674\"), #orders.o_orderdate < Date32(\"8766\")]\


so lovely to see those filters pushed down ❤️

viirya · 2021-11-28T17:18:00Z

Thanks @alamb @Dandandan ! Yeah, if I find some time later to (or someone can) add tests for the other TPCH queries, we can move the test together.

Skip Filter.

f07cbdd

github-actions bot added the datafusion label Nov 26, 2021

Dandandan reviewed Nov 27, 2021

View reviewed changes

Dandandan approved these changes Nov 27, 2021

View reviewed changes

alamb reviewed Nov 27, 2021

View reviewed changes

Add tpch q10 query plan test.

59f724a

viirya added 2 commits November 28, 2021 00:40

split_members should be able to split aliased predicate.

736d27b

Revert "Skip Filter."

7cdd202

This reverts commit f07cbdd.

viirya changed the title ~~Skip adding aliases for Filter~~ split_members should be able to split aliased predicate Nov 28, 2021

Fix clippy.

d95e3f7

Dandandan reviewed Nov 28, 2021

View reviewed changes

alamb mentioned this pull request Nov 28, 2021

Add tests for TPCH explain plans to avoid regressions #1377

Closed

alamb approved these changes Nov 28, 2021

View reviewed changes

alamb changed the title ~~split_members should be able to split aliased predicate~~ Fix Predicate Pushdown: split_members should be able to split aliased predicate Nov 28, 2021

Dandandan self-requested a review November 28, 2021 18:39

Dandandan approved these changes Nov 28, 2021

View reviewed changes

Dandandan merged commit 15db291 into apache:master Nov 28, 2021

houqp added the bug Something isn't working label Nov 29, 2021

alamb mentioned this pull request Nov 30, 2021

Consolidate ConstantFolding and SimplifyExpression #1375

Merged

houqp added the performance Make DataFusion faster label Dec 2, 2021

Fix Predicate Pushdown: split_members should be able to split aliased predicate #1368

Fix Predicate Pushdown: split_members should be able to split aliased predicate #1368

Conversation

viirya commented Nov 26, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

viirya commented Nov 26, 2021

Uh oh!

Dandandan Nov 27, 2021

Choose a reason for hiding this comment

Uh oh!

Dandandan commented Nov 27, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 27, 2021

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 27, 2021

Uh oh!

viirya commented Nov 28, 2021

Uh oh!

viirya commented Nov 28, 2021

Uh oh!

Dandandan Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Nov 28, 2021

Choose a reason for hiding this comment

Uh oh!

viirya commented Nov 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viirya commented Nov 28, 2021 •

edited

Loading